Skip to content

Conversation

CTTY
Copy link
Contributor

@CTTY CTTY commented Jun 10, 2025

Which issue does this PR close?

Related Issues:

What changes are included in this PR?

  • Added implementations of TransactionAction
    • UpdateLocationAction
    • UpdatePropertiesAction
    • UpgradeFormatVersionAction
  • Added as_any to TransactionAction trait
  • Added do_commit in Transaction to commit applied actions

We will also need to implement TransactionAction for FastAppendAction and ReplaceSortOrderAction, and I'm planning to do that in a separate PR.

Are these changes tested?

Added unit tests

pub(crate) trait TransactionAction: Sync + Send {
/// Returns the action as [`Any`] so it can be downcast to concrete types later
fn as_any(self: Arc<Self>) -> Arc<dyn Any>;

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added this mainly for testing/inspecting the content of applied TransactionAction s

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could be a provided method.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I played with this a little bit but it seems like this has to be done within concrete implementations. With code like below:

    fn as_any(self: Arc<Self>) -> Arc<dyn Any> {
        self
    }

And this will fail to the error:

   |
43 |         self
   |         ^^^^ doesn't have a size known at compile-time

It seems that even if self is an Arc<T>, the contained T still has to be Sized to be cast to Any in a provided method. Adding where Self: Sized bound to the method directly will make it uncallable on Arc<dyn TxAction> so it won't work as well

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about using as_any? This way we don't need to implement this for each action:

pub(crate) trait TransactionAction: AsAny {
...
}

Copy link
Contributor Author

@CTTY CTTY Jun 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Renjie, I gave it a try and it's working fine except some syntax sugar to use it on Arc:

        let action = (*tx.actions[0]) // this
            .downcast_ref::<UpdateLocationAction>()
            .unwrap();

This is still much cleaner than using Any directly!

Copy link
Contributor

@liurenjie1024 liurenjie1024 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @CTTY for this pr, generally looks good!

pub(crate) trait TransactionAction: Sync + Send {
/// Returns the action as [`Any`] so it can be downcast to concrete types later
fn as_any(self: Arc<Self>) -> Arc<dyn Any>;

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could be a provided method.

CTTY and others added 2 commits June 12, 2025 11:18
pub(crate) trait TransactionAction: Sync + Send {
/// Returns the action as [`Any`] so it can be downcast to concrete types later
fn as_any(self: Arc<Self>) -> Arc<dyn Any>;

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about using as_any? This way we don't need to implement this for each action:

pub(crate) trait TransactionAction: AsAny {
...
}

Copy link
Contributor

@liurenjie1024 liurenjie1024 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @CTTY for this pr, LGTM!

@liurenjie1024 liurenjie1024 merged commit bcd1033 into apache:main Jun 14, 2025
18 checks passed
@CTTY CTTY deleted the ctty/tx-impl-1 branch June 14, 2025 04:24
}

/// Update table's property.
pub fn set_properties(mut self, props: HashMap<String, String>) -> Result<Self> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ooops, it's an interface change, one minor suggestion, better to call it out in PR description and later release notes. :)
It actually breaks moonlink compilation :(

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, we are making TransactionActions retryable and had to change the existing APIs. There are more API changes coming for other actions like ReplaceSortOrderAction(#1441) and FastAppendAction(WIP)

We definitely need to call this out in the next release notes. cc: @liurenjie1024

I'm not very familiar with moonlink and here goes the dumb question: Would it be better for moonlink to depend on a released/stable iceberg-rs version instead of tracking an unreleased git rev? You can set up a non-blocking github CI to track iceberg-rs/main if you want to catch compatibility issues early

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We definitely need to call this out in the next release notes.

Thanks!

Copy link
Contributor

@dentiny dentiny Jun 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not very familiar with moonlink and here goes the dumb question: Would it be better for moonlink to depend on a released/stable iceberg-rs version instead of tracking an unreleased git rev?

That's the ideal case, but I think the interval between iceberg-rust releases are too long.

You can set up a non-blocking github CI to track iceberg-rs/main if you want to catch compatibility issues early

It's not too much of an overhead as of now, because I sync iceberg-rust upstream on weekly basic.

Copy link
Contributor Author

@CTTY CTTY Jun 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The release interval certainly is an issue 😄

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @dentiny for this comment. I thought about keeping the original api as deprecated, but the concern was to the overhead of maintaining it. But I do agree that we should shoutout this breaking change when release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants